Add container meta info on module level #2682

ruflin · 2016-10-05T07:56:45Z

Most of the docker module metric sets have kind of meta information about the container which is identical across the MetricSets. Until now this information was nested inside each MetricSet which made it very hard to query for all information related to one container. To make querying possible this information was moved to the module level.

Each MetricSet can add information to the module level by adding information to the event with the _module key. This convention is based on the assumption that this key is never used for any other field. During processing of the event this information is moved up to the module level.

It must be made sure that the mapping for this data is added to the fields.yml on the module level. In case of the docker module this is not necessary as the container MetricSet already has the mappings for these values.

An example event looks as following. This makes querying based on docker.container.id simple.

{
    "@timestamp": "2016-05-23T08:05:34.853Z",
    "docker": {
        "container": {
            "id": "2d82c01e5e8a975e8a2912f5dcd7a03ec1505f2c7d2fcf54961582d8e9e34509",
            "name": "admiring_ptolemy",
            "socket": "unix:///var/run/docker.sock"
        },
        "cpu": {
            "usage": {
                "kernel_mode": 0,
                "per_cpu": {
                    "0": 0,
                    "1": 0
                },
                "total": 0,
                "user_mode": 0
            }
        }
    },
    "metricset": {
        "host": "localhost",
        "module": "docker",
        "name": "cpu",
        "rtt": 115
    },
    "type": "metricsets"
}

Additional Changes:

Set default time period to 10s
Make event generation in test based on events builder to make sure events are the same as sent
Remove container info from fields.yml files as not needed anymore. Update mappings.

tsg · 2016-10-05T08:01:37Z

This is great 👍

tsg · 2016-10-05T08:04:17Z

Implementation wise, we might at some point be more explicit about it, like returning two dicts from the Fetch methods. But I think this is fine for now.

ruflin · 2016-10-05T08:10:29Z

@tsg This is currently fully based on a convention to not break existing MetricSets. We could return two objects or follow the approach from @urso in outputs with the Data struct: https://github.com/elastic/beats/blob/master/libbeat/outputs/outputs.go#L19

I would also like to see that the logic for the enrichment would be on the module level. Means we would have for example in the Docker module a method AddMetaInfo that has the logic inside to add this meta data based on the values provided by the MetricSets. This could allow more complex cases and not every MetricSet would have to add it to the event itself. This is very similar to the processors we discussed. Perhaps a module is also a Processor? ... Just some thoughts.

tsg · 2016-10-05T10:56:50Z

metricbeat/beater/event.go

 		},
 	}

+	// In case meta data exists, it is added on the module level
+	if ok {


The use of the ok variable name is a bit confusing here, because it's set more than a few lines before. Perhaps metadataExists, or something like that?

I renamed it to moduleData and moduleDataExists to make it sure it does not get confused with the global meta data.

ruflin · 2016-10-05T12:00:08Z

@tsg I changed the convention to _module as the other one already conflicted in the system module. _module is also more obvious when modifying the metricset. I pushed a new version and also updated the PR docs to not forget about it.

andrewkroh · 2016-10-05T11:55:03Z

metricbeat/beater/event.go

@@ -15,31 +15,31 @@ const (
 // eventBuilder is used for building MetricSet events. MetricSets generate a


s/eventBuilder/EventBuilder/

andrewkroh · 2016-10-05T11:55:21Z

metricbeat/beater/event.go

+	Host          string
+	StartTime     time.Time
+	FetchDuration time.Duration
+	Event         common.MapStr
 	fetchErr      error
 	filters       *processors.Processors
 	metadata      common.EventMetadata
 }

 // build builds an event from MetricSet data and applies the Module-level


s/build/Build/

andrewkroh · 2016-10-05T12:01:35Z

metricbeat/beater/event.go

+	Host          string
+	StartTime     time.Time
+	FetchDuration time.Duration
+	Event         common.MapStr
 	fetchErr      error
 	filters       *processors.Processors
 	metadata      common.EventMetadata
 }

 // build builds an event from MetricSet data and applies the Module-level


Consider diff'ing a before and after golint ./... | grep -v vendor output to look for new warnings.

ugh, haven't checked golint for quite some time :-( Looks like Metricbeat needs some cleanup, especially the docker module. Will do that in an other PR (after fixing the parts of this PR).

I added a TODO on the list here for this one: #2629

andrewkroh · 2016-10-05T12:02:39Z

metricbeat/beater/event.go

-		"type":       typeName,
+	// Checks if additional meta information is provided by the MetricSet under the key _module
+	// This is based on the convention that each MetricSet can provide module data under the key _module
+	moduleData, moudleDataExists := event["_module"]


Consider making _module an exported constant. It might need to be in the mb package in order for MetricSet implementation to use it.

Good idea, I introduce mb.MODULE_DATA. This will simplify changes in the future.

MODULE_DATA isn't a conventional Go name. Even constants should be mixed case.

renamed to ModuleData

andrewkroh · 2016-10-05T12:04:44Z

metricbeat/beater/event.go

 		},
 	}

+	// In case meta data exists, it is added on the module level
+	if moudleDataExists {
+		event[b.ModuleName].(common.MapStr).Update(moduleData.(common.MapStr))


You should test if it's a common.MapStr otherwise this could cause a panic.

I added a validation, but as the common.MapStr is assinged just a few lines before I think it could also be assume that this should never panic.

I was referring to testing that moduleData is a common.MapStr. I didn't see where that was validated.

Ah, got it. Will check that one instead ;-)

tsg · 2016-10-05T12:27:11Z

I like the _module convention more.

@timestamp

Most of the docker module metric sets have kind of meta information about the container which is identical across the MetricSets. Until now this information was nested inside each MetricSet which made it very hard to query for all information related to one container. To make querying possible this information was moved to the module level. Each MetricSet can add information to the module level by adding information to the event with the `_module` key. This convention is based on the assumption that this key is never used for any other field. During processing of the event this information is moved up to the module level. It must be made sure that the mapping for this data is added to the fields.yml on the module level. In case of the docker module this is not necessary as the container MetricSet already has the mappings for these values. An example event looks as following. This makes querying based on `docker.container.id` simple. ``` { "@timestamp": "2016-05-23T08:05:34.853Z", "docker": { "container": { "id": "2d82c01e5e8a975e8a2912f5dcd7a03ec1505f2c7d2fcf54961582d8e9e34509", "name": "admiring_ptolemy", "socket": "unix:///var/run/docker.sock" }, "cpu": { "usage": { "kernel_mode": 0, "per_cpu": { "0": 0, "1": 0 }, "total": 0, "user_mode": 0 } } }, "metricset": { "host": "localhost", "module": "docker", "name": "cpu", "rtt": 115 }, "type": "metricsets" } ``` Additional Changes: * Set default time period to 10s * Make event generation in test based on events builder to make sure events are the same as sent * Remove container info from fields.yml files as not needed anymore. Update mappings. * Fix . mapping in memory.rss to be compatible with elasticsearch < 2.4

ruflin added review Metricbeat Metricbeat labels Oct 5, 2016

ruflin force-pushed the mb-container-meta branch from 702f3da to fe1a5d1 Compare October 5, 2016 07:57

ruflin force-pushed the mb-container-meta branch 2 times, most recently from 5ef4d83 to 81fdc2c Compare October 5, 2016 08:42

tsg reviewed Oct 5, 2016

View reviewed changes

ruflin force-pushed the mb-container-meta branch from 81fdc2c to f425c8d Compare October 5, 2016 11:58

andrewkroh approved these changes Oct 5, 2016

View reviewed changes

ruflin force-pushed the mb-container-meta branch 2 times, most recently from d88a365 to d8431ac Compare October 5, 2016 12:21

ruflin force-pushed the mb-container-meta branch from d8431ac to bee093b Compare October 5, 2016 13:10

ruflin added 3 commits October 5, 2016 16:41

Add review feedback

8c06ee8

introduce constand MODULE_DATA

a5f1d73

ruflin force-pushed the mb-container-meta branch from 4e4ee08 to a5f1d73 Compare October 5, 2016 14:42

Rename constant

8a9bdf3

andrewkroh merged commit cc989a4 into elastic:master Oct 5, 2016

monicasarbu deleted the mb-container-meta branch October 5, 2016 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add container meta info on module level #2682

Add container meta info on module level #2682

ruflin commented Oct 5, 2016 •

edited

Loading

tsg commented Oct 5, 2016

tsg commented Oct 5, 2016

ruflin commented Oct 5, 2016

tsg Oct 5, 2016

ruflin Oct 5, 2016

ruflin commented Oct 5, 2016

andrewkroh Oct 5, 2016

ruflin Oct 5, 2016

andrewkroh Oct 5, 2016

ruflin Oct 5, 2016

andrewkroh Oct 5, 2016

ruflin Oct 5, 2016

ruflin Oct 5, 2016

andrewkroh Oct 5, 2016 •

edited

Loading

ruflin Oct 5, 2016

andrewkroh Oct 5, 2016

ruflin Oct 5, 2016

andrewkroh Oct 5, 2016

ruflin Oct 5, 2016

andrewkroh Oct 5, 2016

ruflin Oct 5, 2016

tsg commented Oct 5, 2016

		@@ -15,31 +15,31 @@ const (
		// eventBuilder is used for building MetricSet events. MetricSets generate a

Add container meta info on module level #2682

Add container meta info on module level #2682

Conversation

ruflin commented Oct 5, 2016 • edited Loading

tsg commented Oct 5, 2016

tsg commented Oct 5, 2016

ruflin commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsg commented Oct 5, 2016

ruflin commented Oct 5, 2016 •

edited

Loading

andrewkroh Oct 5, 2016 •

edited

Loading